Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Azure Linux support #87

Closed
wants to merge 1 commit into from

Conversation

SeanDougherty
Copy link

@SeanDougherty SeanDougherty commented Jun 13, 2024

This PR adds support for Azure Linux to the 0.1.1 version of azure-init. I need to rebase to the current head of main, while I do this, I'm marking the PR as draft to facilitate discussion.

A notable delta between distros is the addition of the "wheel" and "sudo" groups and removal of the other groups in useradd.

Additionally, this PR adds the os-release crate to determine the OS azure-init is operating on during provisioning time.

Finally, Functional tests for AzureLinux are missing. An update of the functional test to support either Azure Linux OR Ubuntu is a goal. I'm happy to make this change to the functional test in this PR or in a separate PR at the maintainer's request.

Copy link
Collaborator

@dongsupark dongsupark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating the PR. Looks nice.
It is up to you to decide whether to write functional tests in this PR or not.

See below:

@@ -26,10 +27,78 @@ impl Distribution for Distributions {
password: &str,
) -> Result<i32, String> {
match self {
Distributions::AzureLinux => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As being implemented in the other PR #86, we should not check for each distro, to be flexible and scalable.
I will soon try to update the PR to make it reviewable. Once it got merged, I hope this PR could be rebased and written without the distro checks.

} else {
let input = format!("{}:{}", username, password);

let mut output = Command::new("chpasswd")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am not an expert of Azure Linux, but is there a specific reason why you need to do password-based authentication?

Previously we had the password authentication in the code base, but the PR #57 removed that part mainly for security. Though there still seems to be cases where password authentication would be necessary. I am just curious what the situation of Azure Linux would be.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually, I believe we do want to eliminate password auth, this was a hold over from me re-implementing the debian logic of an older branch 👍

Thank you for the catch

@t-lo
Copy link
Collaborator

t-lo commented Jun 17, 2024

Hello @SeanDougherty 👋

Thank you for your work, great to see azure-init is useful for you! Just out of curiosity, could you please have a look at #86 and check if that PR "magically" adds Azure-Linux support? We're trying to be distro-agnostic going forward, and it might well be Dongsu's PR will implicitly add support for Azure Linux (and a number of other distros, too).

If you need password authentication support for some reason (see Dongsu's question) I'm sure we'll find a way to work this into the generic approach.

@SeanDougherty
Copy link
Author

Closing b/c PR is outdated. My new approach will be to work alongside maintainers within PR reviews and GH issues to bring Azure Linux support onboard 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants